questions since it's still new and somewhat complicated to follow to me

@IceTDrinker partially reviewed 19 files and all commit messages, and made 26 comments.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

tfhe-zk-pok/src/curve_api/msm.rs line 55 at r2 (raw file):

impl MsmAffine for super::bls12_446::G1Affine {
    type Config = crate::curve_446::g1::Config;
    fn to_ark_affine(&self) -> &Affine<Self::Config> {

#[inline] could help for both, or just say to the compiler you expect the function to disappear

tfhe-zk-pok/src/curve_api/msm.rs line 67 at r2 (raw file):

}

fn compute_window<Aff: MsmAffine>(

#[track_caller] maybe ?

tfhe-zk-pok/src/curve_api/msm.rs line 218 at r2 (raw file):

fn msm_wnaf<A: MsmAffine>(bases: &[A], scalars: &[super::bls12_446::Zp]) -> Projective<A::Config> {
    let num_bits = 299usize;

unclear to me if that's true for both G1 and G2...

tfhe-zk-pok/src/curve_api/msm.rs line 227 at r2 (raw file):

    let lowest = *window_sums.first().unwrap();

    // We're traversing windows from high to low.

comments not useful ?

tfhe-zk-pok/src/curve_api/msm.rs line 405 at r2 (raw file):

        let chunk_results: Vec<SerializableG2Projective> = inputs
            .into_par_iter()
            .map(par_fn!(g2_msm_chunk_worker))

I feel like I'm missing the execute_async stuff from the previous PR ? not sure why

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 41 at r2 (raw file):

#[cfg(feature = "zk-pok")]
#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed

a bit confused as to why this would be needed ? The return type looks very simple ?

Also even if not multi threaded, no need for Send for consistency ?

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 42 at r2 (raw file):

#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed
pub async fn register_cross_origin_coordinator(coordinator_url: &str) -> Result<(), JsValue> {

would it be worth calling it _from_main or something like that ?

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 56 at r2 (raw file):

#[cfg(feature = "zk-pok")]
#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed

same confusion on my end

tfhe-zk-pok/Cargo.toml line 27 at r2 (raw file):

num-bigint = "0.4.5"
tfhe-versionable = { version = "0.7.0", path = "../utils/tfhe-versionable" }
[target.'cfg(target_family = "wasm")'.dependencies]

not sure we want this, since the wasm environment in which it can work is a browser only ?

tfhe/web_wasm_parallel_tests/worker.js line 720 at r2 (raw file):

          if (!supportsThreads) {
            common_bench_str += "_cross_origin";

likely breaks bench parsing/uploading to be checked with @soonum

ci/webdriver.py line 151 at r2 (raw file):

        # Needed for wasm-par-mq sync executor mode
        self.options.add_argument("--headless=new")
        self.options.add_argument("--enable-features=ServiceWorker")

compatible with both chrome and firefox ?

tfhe/docs/integration/js-on-wasm-api.md line 10 at r2 (raw file):

* Node.js: For use in Node.js applications or packages
* Web: For use in web browsers

I think the original web target has been replaced by the one with parallelism from par-mq in the Makefile ?

tfhe/docs/integration/js-on-wasm-api.md line 103 at r2 (raw file):

Standard multi-threading in WASM requires [cross-origin isolation](https://web.dev/articles/cross-origin-isolation-guide) (COOP/COEP headers) to enable `SharedArrayBuffer`. Some deployment environments cannot set these headers (e.g., when embedding in third-party pages or certain hosting platforms).

**TFHE-rs** provides an alternative parallelism mode that works **without cross-origin isolation** by using a Service Worker coordinator and a message-passing–based worker pool. This mode is currently used to accelerate **zero-knowledge proof** computation in the browser.

currently used ? seems a bit odd worded like that, it's "just" a capability of the crate ?

was this doc generated by an LLM first ?

tfhe/docs/integration/js-on-wasm-api.md line 193 at r2 (raw file):

    // Fallback: cross-origin worker pool
    await init_cross_origin_worker_pool(wasmUrl, bindgenUrl, "/sw.js");
}

missing imports like initThreadPool and init_cross_origin_worker_pool ?

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

* `make build_node_js_api` for the Node.js API
* `make build_web_js_api` for the browser API (also used for cross-origin parallelism)

ah so it supports both modes ? means we won't need a new package after all ?

tfhe/web_wasm_parallel_tests/sw.js line 1 at r2 (raw file):

import { setupCoordinator } from "./pkg/coordinator.js";

this is run on the user browser, should this import not be something like an import from the tfhe package ?

tfhe/Cargo.toml line 90 at r2 (raw file):

wasm-bindgen-rayon = { version = "1.3.0", optional = true }
js-sys = { workspace = true, optional = true }
wasm-bindgen-futures = { version = "0.4.56", optional = true }

should be workspaced, looks common to at least wasm-par-mq

tfhe/web_wasm_parallel_tests/index.js line 26 at r2 (raw file):

    // Register the coordinator Service Worker for cross-origin parallelism
    console.info("Running in cross-origin worker mode");
    await init();

is it the init from the wasm-par-mq ?

tfhe/web_wasm_parallel_tests/package.json line 7 at r2 (raw file):

  "main": "index.js",
  "scripts": {
    "build": "cp -r ../../tfhe/pkg ./ && webpack build ./index.js --mode production -o dist --output-filename index.js && cp index.html dist/ && cp favicon.ico dist/ && cp -r pkg dist/ && cp sw.js dist/",

the server needs the full tfhe JS package ?

tfhe-zk-pok/src/curve_api.rs line 364 at r2 (raw file):

    fn multi_mul_scalar(bases: &[Self::Affine], scalars: &[bls12_446::Zp]) -> Self {
        // wnaf overhead seems to not be worth it outside of wasm
        #[cfg(target_family = "wasm")]

not sure if we can still use the wasm family given the constraints of wasm-par-mq as indicated elsewhere

.github/workflows/benchmark_wasm_client_common.yml line 161 at r2 (raw file):

          BROWSER: ${{ matrix.browser }}

      - name: Run benchmarks (cross origin)

need an update in the grafana dashboard ?

Makefile line 688 at r2 (raw file):

.PHONY: build_web_js_api # Build the js API targeting the web browser
build_web_js_api: install_wasm_pack

so the web js API is the one you choose to be the cross origin friendly package ?

Makefile line 692 at r2 (raw file):

	RUSTFLAGS="$(WASM_RUSTFLAGS)" wasm-pack build --release --target=web \
		-- --features=boolean-client-js-wasm-api,shortint-client-js-wasm-api,integer-client-js-wasm-api,zk-pok,extended-types
	cp utils/wasm-par-mq/js/coordinator.js tfhe/pkg/

is there a way to have some sort of build step/script in wasm-bindgen ?

tfhe/web_wasm_parallel_tests/serve.cross-origin.json line 10 at r2 (raw file):

        {
          "key": "Content-Security-Policy",
          "value": "script-src 'self' 'wasm-unsafe-eval' blob:;"

is this blob requirement ok for BC or general good web practices ?

tfhe-zk-pok/src/serialization.rs line 853 at r2 (raw file):

pub type SerializableG1Projective = SerializableProjective<SerializableFp>;
pub type SerializableG2Projective = SerializableProjective<SerializableFp2>;

is it desirable to share the SerializableProjective among the two types ?

unclear on the implications, otherwise having two separate types is fine by me if it makes sense

@IceTDrinker reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

Makefile line 575 at r3 (raw file):

		-p tfhe-test-vectors -- --no-deps -D warnings

.PHONY: clippy_all # Run all clippy targets

let's maybe add a comment in ALL CAPS above here to not forget when adding clippy to also put it in the pcc batch, or we should make a make target that checks clippy_all recipes are all called in the batches (can be done in a later PR)

tfhe-zk-pok/Cargo.toml line 28 at r3 (raw file):

tfhe-versionable = { version = "0.7.0", path = "../utils/tfhe-versionable" }
[target.'cfg(target_family = "wasm")'.dependencies]
getrandom = { version = "0.2", features = ["js"] }

it's workspaced normally

@nsarlin-zama made 24 comments and resolved 4 discussions.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

Makefile line 688 at r2 (raw file):

The build for the cross origin mode does not require any specific flag like the multithreaded one, so I grouped sequential and cross origin parallel in the same target

Ideally, I'd rename the web_js_api_parallel to web_js_api_multithreaded since the base target now support some parallelism, but I'm afraid this would break a lot of things

Makefile line 692 at r2 (raw file):

I will look at that

.github/workflows/benchmark_wasm_client_common.yml line 161 at r2 (raw file):

yes

ci/webdriver.py line 151 at r2 (raw file):

it's chrome specific
fixed

tfhe/Cargo.toml line 90 at r2 (raw file):

done

tfhe/docs/integration/js-on-wasm-api.md line 10 at r2 (raw file):

The way I see it, you can do sequential or cross origin parallelism, they both happen to use the same package. I regrouped in the Makefile them because they were doing effectively the same thing.
I reworded this.

tfhe/docs/integration/js-on-wasm-api.md line 103 at r2 (raw file):

Yes I used an LLM to help me write this, I forgot to mention it because I used it after I wrote the pr description.

"currently" here means that it might be used to parallelize more stuff in the future, but I will reword this sentence if it feels weird.

tfhe/docs/integration/js-on-wasm-api.md line 193 at r2 (raw file):

yes indeed

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

I think the published one is the one with the atomics (currently called parallel ?).
The thing that does not work is using the package compiled with atomics without COOP/COEP. So we need at least:

• 1 package for node
• 1 package for multithreaded/COOP/SharedArrayBuffer
• 1 package for sequential and cross origin parallelism (can be the same package because they require the same options)

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 41 at r2 (raw file):

This looks simple but somehow when it goes through wasm-bindgen and wasm-bindgen-futures, a RefCell is added that makes it !Send.

Clippy complains without the allow because in the general case, it makes the API less usable. For example, this function would not be usable in a tokio executor because it's multithreaded and requires Send futures. But here we don't care about that because this function is made to be called in a JS executor that is single threaded.

So I think silencing this warning is the best thing to do here

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 42 at r2 (raw file):

For me it would imply that there is a similar function that does not need to be called from main if we do this

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 56 at r2 (raw file):

see my comment above

tfhe/web_wasm_parallel_tests/index.js line 26 at r2 (raw file):

no this is a init generated by wasm-bindgen that we need to use before calling any function from tfhe-rs. We also use it in the other examples.

tfhe/web_wasm_parallel_tests/sw.js line 1 at r2 (raw file):

no since this is a service worker, and sadly I did not find any way to make it work directly from the tfhe package like I did for the other workers. It needs a specific js file.

tfhe/web_wasm_parallel_tests/worker.js line 720 at r2 (raw file):

Yes I will update grafana once this is ready

tfhe-zk-pok/Cargo.toml line 27 at r2 (raw file):

I'm gonna check, but I don't think there is an issue in building it anyways for wasm targets, as long as you don't use it. How would you gate it?

tfhe-zk-pok/Cargo.toml line 28 at r3 (raw file):

done

tfhe-zk-pok/src/serialization.rs line 853 at r2 (raw file):

I'm not sure what would be the benefits of having different types ?
This pattern is already used for SerializableAffine

tfhe-zk-pok/src/curve_api/msm.rs line 55 at r2 (raw file):

done

tfhe-zk-pok/src/curve_api/msm.rs line 67 at r2 (raw file):

done

tfhe-zk-pok/src/curve_api/msm.rs line 218 at r2 (raw file):

yes, 299 is the size of the scalars, since it's the scalar that are decomposed in the wnaf. And they are the same for G1 and G2

tfhe-zk-pok/src/curve_api/msm.rs line 227 at r2 (raw file):

I felt like they were just describing the code but said nothing about why things are done like that or the general idea of the algorithm

tfhe-zk-pok/src/curve_api/msm.rs line 405 at r2 (raw file):

This is not strictly needed here because in the example the proof is directly run from a worker, we only needed if we want to run it from the main thread since it cannot block.

I fixed the pr to add this possibility, this gives users more flexibility if they don't want to manager their own workers.

There is a small perf impact since it means more data round trips.

tfhe/web_wasm_parallel_tests/serve.cross-origin.json line 10 at r2 (raw file):

waiting for input from them. I thing it is because blob url is the main reason why they want to reduce the size of the TFHE wasm bundle AFAIK.
Here removing this would require a slightly more complex deployment scenario for the end user, meaning they would have to copy more files.

@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

Makefile line 575 at r3 (raw file):

sorry, this should be fixed now

@IceTDrinker reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

if that's an option that's not too complicated why not, otherwise, have the tfhe-compat package and tfhe depending on it and re-exporting the compat package ?

@tmontaigu reviewed 12 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, soonum, and SouchonTheo).

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

  let result = BigInt(0);
  for (let i = 0; i < bitLen; i++) {
    result << 1n;

Aren't we missing an assigment here ? result <<= 1n;

@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

Ha yes you're right !
The bad news is that this is copy pasted and the same mistake is present in the code that runs the existing tests 🥶

@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Would it be ok to make the fields of this type pub(crate) for the from/into ? Or a into_raw_parts ?

@nsarlin-zama made 4 comments.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

Makefile line 692 at r2 (raw file):

I don't think it's possible :\

tfhe/src/integer/ciphertext/compact_list.rs line 159 at r5 (raw file):

I removed the need to serialize this (I've lost the original thread in reviewable)

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

fixed

tfhe/web_wasm_parallel_tests/package.json line 7 at r2 (raw file):

this is now simplified a bit using #3383

Last few questions for my understanding, otherwise I think we are good to go

Who did you get a review from ? (at least on the ZK pok crate)

@IceTDrinker partially reviewed 20 files and all commit messages, made 5 comments, and resolved 10 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

so tfhe-compat is considered the name to use ?

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

fixed in the original code this was taken from as well ?

tfhe/src/js_on_wasm_api/js_high_level_api/integers.rs line 1534 at r10 (raw file):

    #[derive(Serialize, Deserialize)]
    #[cfg_attr(dylint_lib = "tfhe_lints", allow(serialize_without_versionize))]
    pub(super) struct SerializableCompactCiphertextListBuilder {

doesn't need to be pub(super) I think ?

the trait implem is enough ?

utils/wasm-par-mq/js/worker_helpers.js line 27 at r10 (raw file):

// Compute worker
// `self` is not defined in Node.js. Skip worker bootstrap here.

also I guess the service worker does not exist in node ?

@nsarlin-zama made 3 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

tfhe/src/js_on_wasm_api/js_high_level_api/integers.rs line 1534 at r10 (raw file):

needs to be pub(super) since builder field in ProofInput is pub(super), and I need that to build it in build_with_proof_packed_async

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

yes

utils/wasm-par-mq/js/worker_helpers.js line 27 at r10 (raw file):

Certainly but this is not what is blocking us here. The issue here is that the code is "top level js" and will run even if we don't call any function from this, so we need to prevent it to run in node

@tmontaigu partially reviewed 30 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, soonum, and SouchonTheo).

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 20 at r12 (raw file):

#[cfg(feature = "zk-pok")]
#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed

I belive allow have a reason argument like #[allow(lint, reason = "why")] it could be a bit better than a comment on the side

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 39 at r12 (raw file):

#[cfg(feature = "zk-pok")]
#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed

I belive allow have a reason argument like #[allow(lint, reason = "why")] it could be a bit better than a comment on the side

@nsarlin-zama made 3 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

I guess, unless you have a better idea ? I'm not very inspired

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 20 at r12 (raw file):

done

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 39 at r12 (raw file):

done

@IceTDrinker reviewed 3 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

I'm fine with it